Migrating the whole project from xcontext to go-belt. #164

xaionaro · 2023-05-20T00:07:19Z

What

Here I do substitution of xcontext to go-belt and signallers. The APIs are mostly compatible, so this was a pretty mechanistic stupid job (could mostly be done with sed).

Caveats:

xcontext binds together three different things: observability API, context and signals (currently used to propagate Pause signal across the application). And it is problematic to make the migration of observability API and signals separately, because if I migrate one thing, it breaks another thing. So I migrate them together in one commit.
In xcontext both Cancel and Pause as processed by the same handler. But after migration Cancel is processed by standard context API, while Pause is processed by additional signal-handler. Because of that it is now no unified API between these two.

Why

TLDR: xcontext is succeeded by go-belt. xcontext existed for some time, there was gathered the feedback and in result go-belt is a better version of similar ideas.

More details

The previous design had few very serious problems:

It was basically a God Object, which binds together multiple things (see the first point in Caveats above) and is not scalable horizontally (the amount of observability tools are limited).
It required to propagate everywhere a custom implementation of context.Context, which causes different inconveniences with necessity to type-cast and do other similar stuff. Also the custom context type started to propagate to public APIs which never was the intention (public APIs should always use only the standard context).
Most importantly: xcontext is on life-support, while go-belt is actually supported.

There were also other known (less serious) problems, and in go-belt all that is solved.

How

The PR consists of 4 commits:

[pkg/signaller] Add a package to handle Pause signal. This commit re-implements the signal propagator for Pause (though it can propagate other signals as well). This package mostly just replicates the related code of xcontext.
Migrate from xcontext to go-belt and signaller. This comment switches the code of contest for both observability and signals from xcontext to go-belt and signaller accordingly. This is mostly mechanistic change, which stupidly changes the code to an equivalent. Sorry for a huge commit, but it is difficult to split without breaking functionality :(
Delete xcontext. In this commit package xcontext is deleted (package perf is kept and moved out of xcontext).
Add verbosity to diagnose mis-signaling. Contains changes I needed to diagnose and fix the second commit.

codecov-commenter · 2023-05-20T00:21:30Z

Codecov Report

Patch coverage: 66.07% and project coverage change: -1.58 ⚠️

Comparison is base (57569d4) 63.37% compared to head (ab3087b) 61.79%.

❗ Current head ab3087b differs from pull request most recent head c13b022. Consider uploading reports for the commit c13b022 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #164      +/-   ##
===========================================
- Coverage    63.37%   61.79%   -1.58%     
===========================================
  Files          165      130      -35     
  Lines        10587     9196    -1391     
===========================================
- Hits          6709     5683    -1026     
+ Misses        3134     2842     -292     
+ Partials       744      671      -73

Flag	Coverage Δ
e2e	`49.52% <54.57%> (+1.00%)`	⬆️
integration	`56.85% <63.51%> (+2.30%)`	⬆️
unittests	`45.65% <46.68%> (-3.93%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmds/admin_server/server/server.go	`0.00% <0.00%> (ø)`
cmds/admin_server/storage/mongo/mongo.go	`8.56% <0.00%> (+0.37%)`	⬆️
pkg/api/event.go	`77.77% <ø> (ø)`
pkg/cerrors/cerrors.go	`12.50% <ø> (ø)`
pkg/event/frameworkevent/framework.go	`75.00% <ø> (ø)`
pkg/event/testevent/test.go	`100.00% <ø> (ø)`
pkg/job/reporting.go	`100.00% <ø> (ø)`
pkg/jobmanager/stop.go	`83.33% <0.00%> (ø)`
pkg/loggerhook/httphook.go	`0.00% <0.00%> (ø)`
pkg/target/locker.go	`100.00% <ø> (ø)`
... and 78 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mimir-d · 2023-05-20T01:00:38Z

this is gonna take a while to review, but i know what it is about

xaionaro · 2023-05-22T13:17:27Z

I've switched the base from main to develop, since the PR contains API-breaking changes.

mimir-d

only reviewed first commit so far

pkg/signaller/broadcast_signal.go

pkg/signaller/ctx_key.go

pkg/signaller/signaller.go

pkg/signaller/is_signaled_with.go

pkg/signaller/with_signal.go

pkg/signals/paused.go

mimir-d

2nd commit reviewed; some styling changes and docs requested

SPAG.md

cmds/admin_server/job/rdb/rdb.go

cmds/admin_server/main.go

pkg/api/api.go

pkg/loggerhook/httphook.go

pkg/runner/test_runner_test.go

mimir-d · 2023-05-23T03:29:55Z

since this touches state resume in a lot of places, id like to see a test with that. maybe a step with sleep 30, sigint the server and resume

that would also show if there's any changes in the log output

mimir-d

reviewed last 2 commits; looks ok

pkg/runner/test_runner.go

pkg/jobmanager/start.go

xaionaro · 2023-05-23T15:28:56Z

I've started to redesign signaling: there was a known and shameful problem I had no time to fix in xcontext, and it correlates with some of the feedback. So I guess I finally need to find time to redesign the thing and fix that :)

Will publish today-tomorrow.

UPDATE: Done. But now integration tests are failing. Looking into it...

xaionaro · 2023-05-23T22:28:20Z

since this touches state resume in a lot of places, id like to see a test with that. maybe a step with sleep 30, sigint the server and resume

I thought we have integration tests, which check this. No?

xaionaro · 2023-05-24T15:03:02Z

Commit "[pkg/signaling] Add a package to handle Pause signal" was rewritten from scratch and requires a full review. In other commits the changes are isolated to minimalistically address the provided feedback.

mimir-d

some changes requested for first commit; specifically that panic bit

pkg/signaling/signaler.go

mimir-d · 2023-05-25T13:54:05Z

very nice job in refactoring the signaling part

mimir-d · 2023-05-25T22:21:17Z

I thought we have integration tests, which check this. No?

yeah, that's fair. we'll also see if it breaks on internal usage

A minimal implementation of a handler to keep the context API for Pause. Signed-off-by: Dmitrii Okunev <[email protected]>

Signed-off-by: Dmitrii Okunev <[email protected]>

xaionaro · 2023-05-29T16:01:06Z

Pushed a new version of the PR. Overall:

"Add a package to handle Pause signal" updated the IsSignaledWith signature.
"Provide shorthands for go-belt Logger" -- a new commit, requires review from scratch.
"Migrate from xcontext to go-belt and signaller" uses the simplified syntax for the logger, rebased to new develop and adapted to new IsSignaledWith function signature.
"Delete xcontext" has basically no change (just rebasing / conflict solving)
"Add verbosity to diagnose mis-signaling" now uses the simplified syntax for the logger.

mimir-d · 2023-05-29T20:10:33Z

thanks for addressing all of the review items; merging

xaionaro force-pushed the migrate/go_belt branch from 5d30af4 to 1b70d59 Compare May 20, 2023 00:08

xaionaro requested a review from mimir-d May 20, 2023 00:14

mimir-d assigned mimir-d and xaionaro and unassigned mimir-d May 20, 2023

xaionaro changed the base branch from main to develop May 22, 2023 13:17

mimir-d requested changes May 23, 2023

View reviewed changes

mimir-d reviewed May 23, 2023

View reviewed changes

pkg/runner/test_runner.go Outdated Show resolved Hide resolved

pkg/jobmanager/start.go Show resolved Hide resolved

xaionaro force-pushed the migrate/go_belt branch 2 times, most recently from 88b0926 to 85160d7 Compare May 23, 2023 21:47

xaionaro force-pushed the migrate/go_belt branch 2 times, most recently from f8e09cd to 7a0af56 Compare May 24, 2023 15:01

xaionaro force-pushed the migrate/go_belt branch from 7a0af56 to dd47da0 Compare May 24, 2023 15:06

mimir-d requested changes May 25, 2023

View reviewed changes

pkg/signaling/signaler.go Outdated Show resolved Hide resolved

pkg/signaling/signaler.go Outdated Show resolved Hide resolved

pkg/signaling/signaler.go Show resolved Hide resolved

pkg/signaling/signaler.go Outdated Show resolved Hide resolved

pkg/signaling/signaler.go Show resolved Hide resolved

xaionaro force-pushed the migrate/go_belt branch 3 times, most recently from e00525a to 1e6e688 Compare May 25, 2023 14:32

xaionaro force-pushed the migrate/go_belt branch 3 times, most recently from 8c4d52c to 0311308 Compare May 29, 2023 15:11

xaionaro force-pushed the migrate/go_belt branch 2 times, most recently from 92d416a to c13b022 Compare May 29, 2023 15:32

xaionaro added 5 commits May 29, 2023 16:49

[pkg/signaling] Add a package to handle Pause signal

d5babee

A minimal implementation of a handler to keep the context API for Pause. Signed-off-by: Dmitrii Okunev <[email protected]>

[pkg/logger] Provide shorthands for go-belt Logger

5955b58

Signed-off-by: Dmitrii Okunev <[email protected]>

Migrate from xcontext to go-belt and signaller

f5b7228

Signed-off-by: Dmitrii Okunev <[email protected]>

Delete xcontext

7f34a8b

Signed-off-by: Dmitrii Okunev <[email protected]>

Add verbosity to diagnose mis-signaling

166dc77

Signed-off-by: Dmitrii Okunev <[email protected]>

xaionaro force-pushed the migrate/go_belt branch from c13b022 to 166dc77 Compare May 29, 2023 15:54

mimir-d approved these changes May 29, 2023

View reviewed changes

mimir-d merged commit b12c692 into develop May 29, 2023

mimir-d deleted the migrate/go_belt branch May 29, 2023 20:10

Migrating the whole project from xcontext to go-belt. #164

Migrating the whole project from xcontext to go-belt. #164

Uh oh!

Conversation

xaionaro commented May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

More details

How

Uh oh!

codecov-commenter commented May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mimir-d commented May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaionaro commented May 22, 2023

Uh oh!

mimir-d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimir-d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimir-d commented May 23, 2023

Uh oh!

mimir-d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xaionaro commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaionaro commented May 23, 2023

Uh oh!

xaionaro commented May 24, 2023

Uh oh!

mimir-d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimir-d commented May 25, 2023

Uh oh!

mimir-d commented May 25, 2023

Uh oh!

xaionaro commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimir-d commented May 29, 2023

Uh oh!

Reviewers

Assignees

Labels

xaionaro commented May 20, 2023 •

edited

Loading

codecov-commenter commented May 20, 2023 •

edited

Loading

mimir-d commented May 20, 2023 •

edited

Loading

xaionaro commented May 23, 2023 •

edited

Loading

xaionaro commented May 29, 2023 •

edited

Loading